Skip to content

Conversation

@tzanko-matev
Copy link
Contributor

@tzanko-matev tzanko-matev commented Oct 2, 2025

The purpose of the refactor was to split current code into files and functions where each file/function is responsible for a single concern. See the design docs in this commit for details. No tests were touched

File-SRP

  • ✅ Step 2 complete: introduced src/logging.rs for one-time logger initialisation and migrated tracing session lifecycle (start_tracing, stop_tracing, is_tracing, flush_tracing, ACTIVE flag) into src/session.rs, with src/lib.rs now limited to PyO3 wiring and re-exports.
  • ✅ Step 3 complete: added src/runtime/mod.rs with focused activation, value_encoder, and output_paths submodules; RuntimeTracer now delegates activation gating, value encoding, and writer initialisation through the façade consumed by session.rs.
  • ✅ Step 4 complete: introduced src/monitoring/mod.rs for sys.monitoring types/caches and src/monitoring/tracer.rs for the tracer trait plus callback dispatch; rewired lib.rs, session.rs, and runtime/mod.rs, and kept a top-level tracer re-export for API stability.
  • ✅ Step 5 complete: split the Python package into dedicated formats.py, session.py, and auto_start.py modules, trimmed api.py to a thin façade, and moved the environment auto-start hook into __init__.py.
  • ✅ Step 6 complete: resolved outstanding Rust TODOs (format validation, argv handling, function id stability), expanded module documentation so cargo doc reflects the architecture, and re-ran just test to confirm the refactor remains green.
  • ✅ Test baseline: just test (nextest + pytest) passes with the UV cache scoped to the workspace; direct cargo test still requires CPython development symbols.

Function-SRP

Stage 0 – Baseline & Guardrails

  • just test (Rust + Python suites) passes; captured run via the top-level recipe.
  • ✅ Generated JSON and binary reference traces from examples/value_capture_all.py; outputs stored in artifacts/stage0/value-capture-json/ and artifacts/stage0/value-capture-binary/.
  • ⏳ Summarise current ActivationController behaviour and frame traversal notes for reviewer context.

Stage 1 – Session Start-Up Decomposition

  • ✅ Step 1 (Rust): Introduced session::bootstrap helpers and refactored start_tracing to delegate directory validation, format resolution, and program metadata collection. Tests remain green.
  • ✅ Step 2 (Python): Extracted _coerce_format, _validate_trace_path, and _normalize_activation_path helpers; added tests covering invalid formats and conflicting paths.

Stage 2 – Frame Inspection & Activation Separation

  • ✅ Step 1: Added runtime::frame_inspector::capture_frame to encapsulate frame lookup, locals/globals materialisation, and reference counting; on_line now delegates to the helper while preserving behaviour.
  • ✅ Step 2: Extended ActivationController with should_process_event/handle_return_event, updated callbacks to rely on them, and removed direct state juggling from RuntimeTracer.

Stage 3 – Value Capture Layer

  • ✅ Step 1: Introduced runtime::value_capture::capture_call_arguments; on_py_start now delegates to it, keeping the function focused on orchestration while reusing frame inspectors.
  • ✅ Step 2: Added record_visible_scope helper and refactored on_line to delegate locals/globals registration through it.

Stage 4 – Return Handling & Logging Harmonisation

  • ✅ Added runtime::logging::log_event to consolidate callback logging across start, line, and return handlers.
  • ✅ Exposed record_return_value in runtime::value_capture and refactored RuntimeTracer::on_py_return to orchestrate activation checks, logging, and value recording.
  • ✅ Extended runtime tests with explicit return capture coverage and activation deactivation assertions.

Stage 5 – Cleanup & Regression Sweep

  • ✅ Audited runtime modules for obsolete inline comments or TODOs introduced pre-refactor; none remained after helper extraction.
  • ✅ Documented the helper module map in design-docs/function-level-srp-refactor-plan.md for contributor onboarding.
  • ✅ Re-ran just test (Rust cargo nextest + Python pytest) to confirm post-cleanup parity.

@tzanko-matev tzanko-matev changed the title Code refactoring - single responsibility principle [Refactor] Single responsibility principle Oct 2, 2025
- Extract logging initialisation into `logging.rs` and update `lib.rs` to call the helper.
- Move session lifecycle logic from `lib.rs` into a new `session.rs`, keeping function signatures untouched and re-exporting via `lib.rs`.
- Update module declarations and adjust imports; verify tests.
- Create `src/runtime/mod.rs` as façade exposing `RuntimeTracer`.
- **Task 3A (Team A)**: Extract activation control into `runtime/activation.rs`, exposing a small struct consumed by the tracer.
- **Task 3B (Team B)**: Extract value encoding routines into `runtime/value_encoder.rs`, providing unit tests and benchmarks.
- **Task 3C (Team C)**: Introduce `runtime/output_paths.rs` to encapsulate format-to-filename mapping and writer initialisation.
- Integrate submodules back into `runtime/mod.rs` sequentially once individual tasks are complete; resolve merge conflicts around struct fields.
- Add `monitoring/mod.rs` hosting shared types (`EventId`, `EventSet`, `ToolId`).
- Split trait and dispatcher logic into `monitoring/tracer.rs`; keep callback registration helpers near the sys.monitoring bindings.
- **Task 4A (Team A)**: Port OnceLock caches and registration helpers.
- **Task 4B (Team B)**: Move `Tracer` trait definition and default implementations, updating call sites in runtime tracer and tests.
- Create `session.py`, `formats.py`, and `auto_start.py` with extracted logic.
- Update `api.py` to delegate to the new modules but maintain backward-compatible imports.
- Adjust `__init__.py` to import from `api` and trigger optional auto-start via the new helper.
- Update Python tests and examples to use the reorganised structure.
- Remove obsolete comments (e.g., `//TODO AI!` placeholders) or move them into GitHub issues.
- Update documentation and diagrams to reflect the new module tree.
- Re-run `just test` and linting for both Rust and Python components; capture trace artifacts to confirm unchanged output format.
Copy link
Member

@alehander92 alehander92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly trusting that it makes sense , i am not very familiar with the current codebase, the design ideas/motivation seems probably good

@tzanko-matev
Copy link
Contributor Author

I wanted to make the size of each function and each file smaller in order to reduce the necessary context for further changes. But as a side-effect I think it made the code much more readable for myself

@tzanko-matev tzanko-matev merged commit efea1b5 into main Oct 3, 2025
2 checks passed
@tzanko-matev tzanko-matev deleted the refactor-vc branch October 3, 2025 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants